Skip to content

feat: Add methods for cozy-app settings and instance settings manipulation#1460

Merged
Ldoppea merged 11 commits intomasterfrom
feat/app-settings
Apr 19, 2024
Merged

feat: Add methods for cozy-app settings and instance settings manipulation#1460
Ldoppea merged 11 commits intomasterfrom
feat/app-settings

Conversation

@Ldoppea
Copy link
Copy Markdown
Contributor

@Ldoppea Ldoppea commented Apr 4, 2024

In our cozy-apps, we often need to handle specific settings for each cozy-app. This includes:

  • io.cozy.settings/io.cozy.settings.instance
  • io.cozy.settings/io.cozy.settings.bitwarden
  • io.cozy.home.settings
  • io.cozy.coachco2.settings
  • io.cozy.mespapiers.settings

In the current code base, all those access are made with different code that can be heterogeneous

We want to mutualize those access into two methods:

  • client.getSetting(slug, key)
  • client.saveAfterFetchSetting(slug, key, value)

getSetting is responsible to retrieve the given key from a specific cozy-app setting

saveAfterFetchSetting is responsible to update the given key for a specific cozy-app setting

Note that saveAfterFetchSetting will first fetch the cozy-app settings before editing and saving them. This prevents from having to pass existing settings as parameter and also this ensure we always inject up to date data into the database

Also, in order to have Reactive methods, this PR also implements the useSetting hook that relies on useQuery and useMutation to keep data up-to-date and to mutate them

Hook usage:

const { value, save, query, mutation} =
  useSetting('instance', 'default_redirection', '<optional default value>')

...

if (hasQueryBeenLoaded(query)) {
  console.log('current default app is', value)
  save('mespapiers/')
}

...

useEffect(() => {
  if (mutationStatus === 'loaded') {
    console.log('default app updated')
  }
}, [mutation])

Get method usage:

const value = await client.getSetting('home', 'default_redirection_snackbar_disabled')

Save method usage:

// with raw value
await client.saveAfterFetchSetting('home', 'default_redirection_snackbar_disabled', false)

// or with an update method
await client.saveAfterFetchSetting('home', 'default_redirection_view_count', currentValue => currentValue + 1)

TODO:

What can be improved:

  • Allow onSuccess and onError callbacks that can be passed to useMutation()

@Ldoppea Ldoppea force-pushed the feat/app-settings branch 4 times, most recently from 6b4ac6c to 4a9b541 Compare April 8, 2024 17:13
@Ldoppea Ldoppea marked this pull request as ready for review April 8, 2024 17:17
@Ldoppea Ldoppea force-pushed the feat/app-settings branch 3 times, most recently from 8092419 to 0af2607 Compare April 9, 2024 08:08
) => {
const query = getQuery(slug)

const currentSettingsResult = await client.query(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be relevant to use afetchQueryAndGetFromState here? This would look more like a useQuery by retrieving the cache if it exists

Copy link
Copy Markdown
Contributor Author

@Ldoppea Ldoppea Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I made the edit this morning but did not pushed it yet seems some discussion are still in progress

I pushed theme here, but CI will fail due to types error: https://github.com/cozy/cozy-client/compare/0af2607eabfdcc3fe08b06f560166123a320b91d..c44f83a86d7b55513e3bdffbbff1ca9edf62cbb6

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(cf #931 et l'autre issue liée pour pourquoi cette méthode pour le moment)

@Ldoppea Ldoppea force-pushed the feat/app-settings branch 4 times, most recently from bd57188 to ef64bdc Compare April 9, 2024 22:33
Ldoppea added 6 commits April 10, 2024 09:32
In our cozy-apps, we often need to handle specific settings for each
cozy-app. This includes:
- io.cozy.settings/io.cozy.settings.instance
- io.cozy.settings/io.cozy.settings.bitwarden
- io.cozy.home.settings
- io.cozy.coachco2.settings
- io.cozy.mespapiers.settings

In the current code base, all those access are made with different code
that can be heterogeneous

We want to mutualize those access into two methods:
- client.getSetting(slug, key)
- client.saveAfterFetchSetting(slug, key, value)

`getSetting` is responsible to retrieve the given `key` from a specific
cozy-app setting

`saveAfterFetchSetting` is responsible to update the given `key` for a
specific cozy-app setting

Note that `saveAfterFetchSetting` will first fetch the cozy-app
settings before editing and saving them. This prevents from having to
pass existing settings as parameter and also this ensure we always
inject up to date data into the database
In previous commit we added `getSetting` and `saveAfterFetchSetting`
methods into cozy-client

Those methods are not Reactive so we always need to call `getSetting`
on demand to have fresh data. Also `saveAfterFetchSetting` will always
fetch data before updating and saving them into database even if the
data is already up-to-date

In order to improve this, this commit implements the `useSetting` hook
that relies on `useQuery` and `useMutation` to keep data up-to-date and
to mutate them
`.query` would return `undefined` if called under the fetch policy's
delay

We want to retrieve the data from local storage in that scenario

So the solution is to call `.fetchQueryAndGetFromState` instead of
`.query`
@Ldoppea Ldoppea force-pushed the feat/app-settings branch from ef64bdc to 18a6c8f Compare April 10, 2024 07:32
@Ldoppea Ldoppea force-pushed the feat/app-settings branch 4 times, most recently from fefd7e3 to e87a674 Compare April 11, 2024 15:51
import fetchPolicies from '../policies'
import { Q } from '../queries/dsl'

const defaultFetchPolicy = fetchPolicies.olderThan(60 * 60 * 1000)
Copy link
Copy Markdown
Member

@zatteo zatteo Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking about this default fetch policy. Maybe it is too long ? I do not have any use case now to explain (😅) but I have the feeling that with 1 hour we can get outdated setting value where we de not want outdated setting value.

Maybe it should be short but enough to avoid 2 requests when doing a getSetting + saveAfterFetchSetting? Like 10 seconds?

Oh you already changed it in a next commit perfect.

Comment thread docs/react-integration.md Outdated
Comment thread packages/cozy-client/src/helpers/settings.js
Comment thread packages/cozy-client/src/helpers/settings.js Outdated
*
* @param {CozyClient} client - Cozy client instance
* @param {string} slug - the cozy-app's slug containing the setting (can be 'instance' for global settings)
* @param {string} key - The name of the setting to retrieve
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe having an array of keys could have been useful

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not, I'll try to add this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have this in my mind since yesterday but I'm struggling on choosing the best API.

Declaration

For the declaration I imagine the following possibilities:

Use same hook for both single string param or multiple array param

const { value } = useSetting('home', 'mysetting')
// value = 'some_value'

const { value } = useSetting('home', ['mysetting1', 'mysetting2'])
// value = {
//   mysetting1: 'some_value_1',
//   mysetting2: 'some_value_2'
// }

Use a different hooks for single and multiple params

const { value } = useSetting('home', 'mysetting')
const { values } = useSettings('home', ['mysetting1', 'mysetting2'])

Use an object instead of an array to declare default values (but I we may remove the default value thing, I think I can forget this option)

const { value } = useSetting('home', 'mysetting')
const { values } = useSettings('home', {
  mysetting1: 'defaultValue1',
  mysetting2': 'defaultValue2'
})

Mutation

Mutation is getting harder to approach

Should we return multiple save methods for each key?

const { value, savemysetting1,  savemysetting2} = useSetting('home', ['mysetting1', 'mysetting2'])

savemysetting1('value1')
savemysetting2('value2')

or

const { value, save } = useSetting('home', ['mysetting1', 'mysetting2'])

save.mysetting1('value1')
save.mysetting2('value2')

This constrains the API and so prevents errors, but sadly this also prevents mutating multiple settings at once

Should we return a save that takes an objet?

const { value, save} = useSetting('home', ['mysetting1', 'mysetting2', 'mysetting3'])

savemysetting1({
  mysetting1: 'value1',
  mysetting3: 'value3'
})

This allow mutating multiple settings at once, but this makes the API less contraint and I'm wondering if we should allow inserting new keys or make a code that throw a specific error is the method is called with keys that are not present in the initial array?

Copy link
Copy Markdown
Contributor Author

@Ldoppea Ldoppea Apr 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a proposal here: dcb23c5

(I still have to update documentation)

Usage would be:

Hook

const { value, save } = useSetting('home', ['mysetting1', 'mysetting2'])
...

// ✅ Valid
save({
  mySetting1: 'newValue1',
  mySetting2: 'newValue2'
})

// ✅ Valid
save({
  mySetting1: 'newValue1'
})

// ❌ Not valid because new key inserted
save({
  mySetting1: 'newValue1',
  mySetting2: 'newValue2',
  mySetting3: 'newValue3'
})

Get

const result = await client.getSetting('home', ['mysetting1', 'mysetting2'])

console.log(result.mysetting1)
console.log(result.mysetting2)

Save

// With raw value
await client.saveAfterFetchSetting(
  'instance',
  {
    some_instance_key: 'some_new_instance_value',
    some_instance_key2: 'some_new_instance_value_2'
  }
)


// With a method
await client.saveAfterFetchSetting(
  'instance',
  currentValue => ({
    ...currentValue,
    some_instance_key: currentValue.some_instance_key + 1,
    some_instance_key2: 'some_new_instance_value_2'
  }),
  ['some_instance_key']
)

When using a method we must pass a setterKeys parameters so the save method can still extract correct values. I would prefer not to have this parameter but I did not find a better way 😕

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I typed those methods to prevent mistakes when using them. Here is how the linter and the auto-complete features behave:
image

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the cozy-home PR with the new multiple-settings API: linagora/cozy-home@e20239d

*
* If the query is called under the fetch policy's delay, then the query
* is not executed and nothing is returned. If you need a result anyway,
* please use `fetchQueryAndGetFromState` instead
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍👍

@Ldoppea Ldoppea force-pushed the feat/app-settings branch from e87a674 to 582f832 Compare April 12, 2024 14:22
Copy link
Copy Markdown
Contributor

@paultranvan paultranvan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work! Hopefully the beginning of future great contributions 😄

@Ldoppea Ldoppea force-pushed the feat/app-settings branch from dcb23c5 to de7a8c6 Compare April 18, 2024 16:31
Ldoppea added a commit to linagora/cozy-home that referenced this pull request Apr 19, 2024
In linagora/cozy-client#1460 we implemented a new `useSettings` hook

This hook can be used in `DefaultRedirectionSnackbar` in order to
simplify code and homogenize the way we access cozy-apps settings
@Ldoppea Ldoppea merged commit af8ee14 into master Apr 19, 2024
@Ldoppea Ldoppea deleted the feat/app-settings branch April 19, 2024 10:21
Ldoppea added a commit to linagora/cozy-home that referenced this pull request Apr 19, 2024
`cozy-client` has been upgraded to `46.9.0` to retrieve the new
useSettings hook

Related PR: linagora/cozy-client#1460
Ldoppea added a commit to linagora/cozy-home that referenced this pull request Apr 19, 2024
In linagora/cozy-client#1460 we implemented a new `useSettings` hook

This hook can be used in `DefaultRedirectionSnackbar` in order to
simplify code and homogenize the way we access cozy-apps settings
Ldoppea added a commit to cozy/cozy-keys-browser that referenced this pull request Apr 19, 2024
`cozy-client` has been upgraded to `46.9.1` to retrieve the new
`getSettings()` and `saveAfterFetchSettings()` methods

Related PR: linagora/cozy-client#1460
Ldoppea added a commit to cozy/cozy-keys-browser that referenced this pull request Apr 19, 2024
`cozy-client` has been upgraded to `46.9.1` to retrieve the new
`getSettings()` and `saveAfterFetchSettings()` methods

Related PR: linagora/cozy-client#1460
zatteo pushed a commit to cozy/cozy-keys-browser that referenced this pull request May 31, 2024
`cozy-client` has been upgraded to `46.9.1` to retrieve the new
`getSettings()` and `saveAfterFetchSettings()` methods

Related PR: linagora/cozy-client#1460
Ldoppea added a commit to linagora/cozy-home that referenced this pull request Sep 16, 2024
In linagora/cozy-client#1460 we implemented a new `useSettings` hook

This hook can be used in `DefaultRedirectionSnackbar` in order to
simplify code and homogenize the way we access cozy-apps settings
Ldoppea added a commit to linagora/cozy-home that referenced this pull request Sep 19, 2024
In linagora/cozy-client#1460 we implemented a new `useSettings` hook

This hook can be used in `DefaultRedirectionSnackbar` in order to
simplify code and homogenize the way we access cozy-apps settings
Ldoppea added a commit to linagora/cozy-home that referenced this pull request Sep 24, 2024
In linagora/cozy-client#1460 we implemented a new `useSettings` hook

This hook can be used in `DefaultRedirectionSnackbar` in order to
simplify code and homogenize the way we access cozy-apps settings
Ldoppea added a commit to linagora/cozy-home that referenced this pull request Jan 20, 2025
In linagora/cozy-client#1460 we implemented a new `useSettings` hook

This hook can be used in `DefaultRedirectionSnackbar` in order to
simplify code and homogenize the way we access cozy-apps settings
paultranvan pushed a commit to linagora/cozy-home that referenced this pull request Feb 12, 2025
In linagora/cozy-client#1460 we implemented a new `useSettings` hook

This hook can be used in `DefaultRedirectionSnackbar` in order to
simplify code and homogenize the way we access cozy-apps settings
Ldoppea added a commit to linagora/cozy-home that referenced this pull request Feb 12, 2025
In linagora/cozy-client#1460 we implemented a new `useSettings` hook

This hook can be used in `DefaultRedirectionSnackbar` in order to
simplify code and homogenize the way we access cozy-apps settings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants